Restore workspace from backup #1572
Conversation
Allda
requested review from
akurinnoy,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Allda The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
A new init container is added to the workspace deployment in case user choose to restore the workspace from backup. By setting workspace attribute "controller.devfile.io/restore-workspace" the controller sets a new init container instead of cloning data from git repository. By default an automated path to restore image is used based on cluster settings. However user is capable overwrite that value using another attribute "controller.devfile.io/restore-source-image". The restore container runs a wokspace-recovery.sh script that pull an image using oras an extract files to a /project directory. Signed-off-by: Ales Raszka <araszka@redhat.com>
A new tests that verifies the workspace is created from a backup. It checks if a deployment is ready and if it contains a new restore init container with proper configuration. There are 2 tests - one focused on common pvc and other that have per-workspace storage. Signed-off-by: Ales Raszka <araszka@redhat.com>
The condition whether an workspace should be restored from workspace was in the restore module itself. This make a reading a code more difficult. Now the condition is checked in the controller itself and restore container is only added when enabled. This commit also fixes few minor changes based on the code review comments: - Licence header - Attribute validation - Add a test for disabled workspace recovery - Typos Signed-off-by: Ales Raszka <araszka@redhat.com>
A new config is added to control the restore container. Default values are set for the new init container. It can be changed by user in the config. The config uses same logic as the project clone container config. Signed-off-by: Ales Raszka <araszka@redhat.com>
Allda
force-pushed
the
23570-restore-workspace
branch
from
1b95d94 to
5480c1c
Compare
|
@Allda : I'm facing a strange issue while testing this functionality on CRC cluster . I've tried on both amd64 and arm64 variants but face same issue. I used samples/plain-workspace.yaml for testing. Everything goes fine till step 4, but when I create the restore backup manifest I can see devworkspace resource is created but there is no corresponding pod for it: oc create -f restore-dw.yaml
devworkspace.workspace.devfile.io/plain-devworkspace created
oc get dw
NAME DEVWORKSPACE ID PHASE INFO
plain-devworkspace workspace612b8ddca9ff45d5 Running Workspace is running
oc get pods
No resources found in rokumar-dev namespace.I had just modified the name from the restore manifest you shared: kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
labels:
controller.devfile.io/creator: ""
name: plain-devworkspace
spec:
started: true
routingClass: 'basic'
template:
attributes:
controller.devfile.io/storage-type: common
controller.devfile.io/restore-workspace: 'true'Could you please check if I'm missing something? |
I am not sure why it doesn't work on your system. I tried the workspace you mentioned and the backup and other pods were created successfully. Are there any logs you can share or see if the workspace has any pods at the very start? |
|
@Allda thank you for the updates,
I ran into some issues with this in my lab cluster where the auth secret isn't mounting properly onto the restore initcontainer, I will investigate more on Monday and provide more feedback |
|
I tested, and I was able to restore workspace from backup using OpenShift Internal registry and Quay.io. Logs of restored workspace ------------------------------------------
📌 Pod: workspacee119c8fcfc124631-57b8699795-k2jl4
🔹 Logs for container: workspace-restore
Restoring devworkspace from image 'default-route-openshift-image-registry.apps-crc.testing/openshift-operators/test-devworkspace-should-get-backup:latest' to path '/projects'
Using mounted service account token for registry authentication
Login Succeeded
Downloading fde49750175c devworkspace-backup.tar.gz
Downloaded fde49750175c devworkspace-backup.tar.gz
Pulled [registry] default-route-openshift-image-registry.apps-crc.testing/openshift-operators/test-devworkspace-should-get-backup:latest
Digest: sha256:f0060e5ca2e3183846cddbadc451c0d2183044aabc98016e468fdb42c4f0a464
./
./web-nodejs-sample/
./web-nodejs-sample/.gitattributes
./web-nodejs-sample/.gitignore
./web-nodejs-sample/LICENSE
./web-nodejs-sample/README.md
./web-nodejs-sample/devfile.yaml
./web-nodejs-sample/package-lock.json
./web-nodejs-sample/package.json
./web-nodejs-sample/.git/
./web-nodejs-sample/.git/description
./web-nodejs-sample/.git/config
./web-nodejs-sample/.git/HEAD
./web-nodejs-sample/.git/packed-refs
./web-nodejs-sample/.git/index
./web-nodejs-sample/.git/FETCH_HEAD
./web-nodejs-sample/.git/branches/
./web-nodejs-sample/.git/hooks/
./web-nodejs-sample/.git/hooks/applypatch-msg.sample
./web-nodejs-sample/.git/hooks/commit-msg.sample
./web-nodejs-sample/.git/hooks/fsmonitor-watchman.sample
./web-nodejs-sample/.git/hooks/post-update.sample
./web-nodejs-sample/.git/hooks/pre-applypatch.sample
./web-nodejs-sample/.git/hooks/pre-commit.sample
./web-nodejs-sample/.git/hooks/pre-merge-commit.sample
./web-nodejs-sample/.git/hooks/pre-push.sample
./web-nodejs-sample/.git/hooks/pre-rebase.sample
./web-nodejs-sample/.git/hooks/pre-receive.sample
./web-nodejs-sample/.git/hooks/prepare-commit-msg.sample
./web-nodejs-sample/.git/hooks/push-to-checkout.sample
./web-nodejs-sample/.git/hooks/sendemail-validate.sample
./web-nodejs-sample/.git/hooks/update.sample
./web-nodejs-sample/.git/info/
./web-nodejs-sample/.git/info/exclude
./web-nodejs-sample/.git/objects/
./web-nodejs-sample/.git/objects/pack/
./web-nodejs-sample/.git/objects/pack/pack-00c9cb529f78a543deaf778dfaffcb012aef3c11.pack
./web-nodejs-sample/.git/objects/pack/pack-00c9cb529f78a543deaf778dfaffcb012aef3c11.rev
./web-nodejs-sample/.git/objects/pack/pack-00c9cb529f78a543deaf778dfaffcb012aef3c11.idx
./web-nodejs-sample/.git/objects/info/
./web-nodejs-sample/.git/refs/
./web-nodejs-sample/.git/refs/heads/
./web-nodejs-sample/.git/refs/heads/main
./web-nodejs-sample/.git/refs/tags/
./web-nodejs-sample/.git/refs/remotes/
./web-nodejs-sample/.git/refs/remotes/origin/
./web-nodejs-sample/.git/refs/remotes/origin/HEAD
./web-nodejs-sample/.git/logs/
./web-nodejs-sample/.git/logs/HEAD
./web-nodejs-sample/.git/logs/refs/
./web-nodejs-sample/.git/logs/refs/remotes/
./web-nodejs-sample/.git/logs/refs/remotes/origin/
./web-nodejs-sample/.git/logs/refs/remotes/origin/HEAD
./web-nodejs-sample/.git/logs/refs/heads/
./web-nodejs-sample/.git/logs/refs/heads/main
./web-nodejs-sample/.github/
./web-nodejs-sample/.github/CODEOWNERS
./web-nodejs-sample/.vscode/
./web-nodejs-sample/.vscode/launch.json
./web-nodejs-sample/app/
./web-nodejs-sample/app/app.js
./.code-workspace
Restore completed successfully.
Logs of restored workspace from Quay.io: |
|
I am running into an issue in this particular scenario, if I have the DWOC with an auth secret to a private repo like so: and if I manually create the auth secret named the workspace pods create and terminated infinitely due to the deployment being updated repeatedly: Screen.Recording.2026-01-27.at.6.14.20.PM.mov@akurinnoy @rohanKanojia have you experienced something similar? I need to investigate the cause/solution a bit more, but the problem occurs because for whatever reason, this situation causes volume mounts' ordering to change repeatedly: |
|
Oops, I'm sorry for the accidental close |
pkg/library/restore/restore.go
Outdated
| MountPath: constants.DefaultProjectsSourcesRoot, | ||
| }, | ||
| } | ||
| registryAuthSecret, err := secrets.HandleRegistryAuthSecret(ctx, k8sClient, workspace.DevWorkspace, workspace.Config, "", scheme, log) |
There was a problem hiding this comment.
Could we refactor HandleRegistryAuthSecret to avoid having the caller pass in "" as a parameter in:
HandleRegistryAuthSecret(ctx, k8sClient, workspace.DevWorkspace, workspace.Config, "", scheme, log)
?
There was a problem hiding this comment.
What do you suggest? null value or a different solution?
There was a problem hiding this comment.
I notice for the restore init container, we are using HandleRegistryAuthSecret to only get the secret, and not copy. But in the backup cron job, we use it for both get and copy:
I was thinking about creating:
func GetNamespaceRegistryAuthSecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace,
dwOperatorConfig *controllerv1alpha1.OperatorConfiguration, operatorConfigNamespace string, scheme *runtime.Scheme, log logr.Logger,
) (*corev1.Secret, error) {
return HandleRegistryAuthSecret(ctx, c, workspace, dwOperatorConfig, "", scheme, log)
}
inside pkg/secrets/backup.go, and use it here:
| registryAuthSecret, err := secrets.HandleRegistryAuthSecret(ctx, k8sClient, workspace.DevWorkspace, workspace.Config, "", scheme, log) | |
| registryAuthSecret, err := secrets.GetNamespaceRegistryAuthSecret(ctx, k8sClient, workspace.DevWorkspace, workspace.Config, scheme, log) |
There was a problem hiding this comment.
I changed the functions based on the suggestion.
A new sample image is created in the CI and is used in the integration tests. Signed-off-by: Ales Raszka <araszka@redhat.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Ales Raszka <araszka@redhat.com>
|
I think I found the cause of #1572 (comment). It seems unrelated to this PR. I will need to gather more evidence, and create a fix |
pkg/library/env/workspaceenv.go
Outdated
| Name: devfileConstants.ProjectsRootEnvVar, | ||
| Value: constants.DefaultProjectsSourcesRoot, | ||
| }) | ||
| if workspace.Config.Workspace.BackupCronJob.OrasConfig != nil { |
There was a problem hiding this comment.
To me it seems like a potential nil pointer issue, when workspace restore is enabled but backup configuration is not set in DWOC.
| if workspace.Config.Workspace.BackupCronJob.OrasConfig != nil { | |
| if workspace.Config.Workspace.BackupCronJob != nil && | |
| workspace.Config.Workspace.BackupCronJob.OrasConfig != nil { |
pkg/secrets/backup.go
Outdated
| err = c.Delete(ctx, existingNamespaceSecret) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| namespaceSecret = &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: constants.DevWorkspaceBackupAuthSecretName, | ||
| Namespace: workspace.Namespace, | ||
| Labels: map[string]string{ | ||
| constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, | ||
| constants.DevWorkspaceWatchSecretLabel: "true", | ||
| }, | ||
| }, | ||
| Data: sourceSecret.Data, | ||
| Type: sourceSecret.Type, | ||
| } | ||
| if err := controllerutil.SetControllerReference(workspace, namespaceSecret, scheme); err != nil { | ||
| return nil, err | ||
| } | ||
| err = c.Create(ctx, namespaceSecret) |
There was a problem hiding this comment.
When multiple workspaces start simultaneously they race to copy the same secret causing failures.
err = c.Delete(ctx, existingNamespaceSecret) // race window opens
// time gap
err = c.Create(ctx, namespaceSecret) // race window closesI think you can update instead of delete and create. Does this make sense?
There was a problem hiding this comment.
I replaced the function with the SyncObjectWithCluster that performs object sync in a standardized way and only syncs it when secrets is changed.
pkg/library/restore/restore.go
Outdated
| MountPath: constants.DefaultProjectsSourcesRoot, | ||
| }, | ||
| } | ||
| registryAuthSecret, err := secrets.HandleRegistryAuthSecret(ctx, k8sClient, workspace.DevWorkspace, workspace.Config, "", scheme, log) |
There was a problem hiding this comment.
Could you please clarify why you passed an empty string for operatorNamespace?
I noticed that the backup implementation retrieves the operator namespace and passes it to the same function.
Sorry if this has already been discussed. If so, please point that out to me.
There was a problem hiding this comment.
@akurinnoy I created a similar discussion here: #1572 (comment)
There was a problem hiding this comment.
I addressed this based on the suggestion.
Signed-off-by: Ales Raszka <araszka@redhat.com>
Signed-off-by: Ales Raszka <araszka@redhat.com>
In case the backup config is not present the value might be null and fails. The new condition handles it. Signed-off-by: Ales Raszka <araszka@redhat.com>
The delay between checking the empty dir and copying a backup content might cause an issue. This fix moves the check right before the content is being copied which minimize the delay. Signed-off-by: Ales Raszka <araszka@redhat.com>
The previous solution always deleted a secret if exist and re-create it. This can lead to potential issues. The new code uses SyncObjectWithCluster that is used across a whole codebase and minimize the risk of issues. Signed-off-by: Ales Raszka <araszka@redhat.com>
Allda
force-pushed
the
23570-restore-workspace
branch
from
50d4dd0 to
27ee785
Compare
|
@dkwon17 I addressed all the code review comments. Is this PR good to be merged? |
| //Role kinds | ||
| Role = "Role" | ||
| // ClusterRole kind | ||
| ClusterRole = "ClusterRole" |
There was a problem hiding this comment.
This seems to be a very generic name and can be mistaken for an API concept or exported constant from Kubernetes (which doesn’t exist).
I suggest renaming it to something like rbacRoleKind / rbacClusterRoleKind, or documenting that this is a literal Kind string used for comparison.
| //Role kinds | |
| Role = "Role" | |
| // ClusterRole kind | |
| ClusterRole = "ClusterRole" | |
| // Role kind | |
| rbacRoleKind = "Role" | |
| // ClusterRole kind | |
| rbacClusterRoleKind = "ClusterRole" |
| func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace, | ||
| dwOperatorConfig *controllerv1alpha1.OperatorConfiguration, operatorConfigNamespace string, scheme *runtime.Scheme, log logr.Logger, | ||
| ) (*corev1.Secret, error) { | ||
| secretName := dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret |
There was a problem hiding this comment.
Potential nil pointer deference:
| secretName := dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret | |
| if dwOperatorConfig.Workspace == nil || | |
| dwOperartorConfig.Workspace.BackupCronJob == nil || | |
| dwOperatorConfig.Workspace.BackupCronJob.Registry == nil { | |
| return nil, fmt.Errorf("backup/restore configuration not properly set in DevWorkspaceOperatorConfig") | |
| } | |
| secretName := dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret |
| // Construct the desired secret state | ||
| desiredSecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: constants.DevWorkspaceBackupAuthSecretName, | ||
| Namespace: workspace.Namespace, | ||
| Labels: map[string]string{ | ||
| constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, | ||
| constants.DevWorkspaceWatchSecretLabel: "true", | ||
| }, | ||
| }, | ||
| Data: sourceSecret.Data, | ||
| Type: sourceSecret.Type, | ||
| } |
There was a problem hiding this comment.
Here seems to be another race condition in secret copying. If multiple workspaces are restoring simultaneously in the same namespace, they will race to create/update the same secret name. Does this make sense?
| // Construct the desired secret state | |
| desiredSecret := &corev1.Secret{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: constants.DevWorkspaceBackupAuthSecretName, | |
| Namespace: workspace.Namespace, | |
| Labels: map[string]string{ | |
| constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, | |
| constants.DevWorkspaceWatchSecretLabel: "true", | |
| }, | |
| }, | |
| Data: sourceSecret.Data, | |
| Type: sourceSecret.Type, | |
| } | |
| // Construct the desired secret state | |
| desiredSecret := &corev1.Secret{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: constants.DevWorkspaceBackupAuthSecretName + "=" + workspace.Status.DevWorkspaceId, | |
| Namespace: workspace.Namespace, | |
| Labels: map[string]string{ | |
| constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, | |
| constants.DevWorkspaceWatchSecretLabel: "true", | |
| }, | |
| }, | |
| Data: sourceSecret.Data, | |
| Type: sourceSecret.Type, | |
| } |
|
Now that I'm testing with updated changes, I'm seeing issue that after creating restored workspace manifest . Restore pod correctly starts However, the DevWorkspace is not able to come out of |
| } | ||
| if workspace.Config.Workspace.ProjectCloneConfig.ImagePullPolicy != "" { | ||
| projectCloneOptions.PullPolicy = config.Workspace.ProjectCloneConfig.ImagePullPolicy | ||
| if restore.IsWorkspaceRestoreRequested(&workspace.Spec.Template) { |
There was a problem hiding this comment.
I see every reconcile checks IsWorkspaceRestoreRequested(), I think this would keep adding restore init container, and the workspace would never reach Running phase.
The restore attribute should be automatically removed after successful completion.
What does this PR do?
Add init container for workspace restoration
A new init container is added to the workspace deployment in case user choose to restore the workspace from backup.
By setting the workspace attribute "controller.devfile.io/restore-workspace" the controller sets a new init container instead of cloning data from git repository.
By default an automated path to restore image is used based on cluster settings. However user is capable overwrite that value using another attribute "controller.devfile.io/restore-source-image".
The restore container runs a wokspace-recovery.sh script that pull an image using oras an extract files to a /project directory.
What issues does this PR fix or reference?
#1525
Is it tested? How?
No automated tests are available in the first phase. I will add tests once I get the first approval that the concept is ok.
How to test:
kubectl delete devworkspace restore-workspace-2controller.devfile.io/restore-workspace)PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheWhat's missing: